Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Going all in on linting #109

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

willpower232
Copy link
Collaborator

I've been using easy-coding-standard to wrap phpcs and php-cs-fixer and radically shorten the configuration which explains all my past confusion around spaces in this project.

I tried to maintain the behaviour of composer lint and composer lint-ci in spite of ecs's default behaviour being to not fix.

I presume that ecs saw the inheritdoc's weren't necessary as everything was typed perhaps? Not sure on that one. phpstan doesn't seem to mind so ¯\_(ツ)_/¯

I added in another linter I use which I think just makes sure it is valid PHP rather than applying a style.

I also spotted that the phpstan config could be simplified.

cc @NicolasCARPi that I can't directly invite for a review via github

@willpower232 willpower232 self-assigned this Jun 4, 2023
Copy link
Sponsor Collaborator

@NicolasCARPi NicolasCARPi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personnally I would chose to stay with PHP-CS-Fixer, as it is a more popular choice, and that's an important parameter when introducing a new dependency. But given that we're talking about a linter, it's not a critical choice, so whatever.

It seems ecs is basically a wrapper for PHP-CS allowing parallel scan which we do not care because the codebase is small and honestly if it takes 200ms or 120ms it's not important.

But if you think it has important features, I mean why not, it's just that I don't immediatly see the advantage of moving to another linter lib instead of tweaking the existing config.

As for [] vs array(), I'm partial to the long form as I find it easier to spot in the code, but it's a matter of taste.

I like that it makes long lines split in multilines.

@willpower232
Copy link
Collaborator Author

ecs wraps both php cs and php cs fixer so technically all I have done is reduce the length of the rules file into one with more groups and no specific rules and made it so the default behaviour is to check and not to fix

@willpower232
Copy link
Collaborator Author

The complexity of the current configuration has required me to remove a specific rule as it is now handled by default and caused a lint failure

image

137df4d

The comparative simplicity of this PR means we aren't beholden to specific minor decisions made upstream which we don't really care about.

@NicolasCARPi
Copy link
Sponsor Collaborator

@willpower232 Do you wish to work on resolving conflicts on this branch, or should this be closed now?

@willpower232 willpower232 marked this pull request as draft April 18, 2024 08:23
@willpower232
Copy link
Collaborator Author

I'll pick this up at some point, probably in a couple of weeks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants